Codecov Report

❌ Patch coverage is 98.41270% with 1 line in your changes missing coverage. Please review.
✅ Project coverage is 85.38%. Comparing base (7353bcd) to head (28385a0).
⚠️ Report is 9 commits behind head on main.

@@            Coverage Diff             @@
##             main     #882      +/-   ##
==========================================
+ Coverage   85.23%   85.38%   +0.14%     
==========================================
  Files          50       50              
  Lines        5317     5337      +20     
  Branches     5317     5337      +20     
==========================================
+ Hits         4532     4557      +25     
+ Misses        565      560       -5     
  Partials      220      220              

☔ View full report in Codecov by Sentry.
📢 Have feedback on the report? Share it here.

I think this is probably fine for now and maybe worth merging as an intermediate. I do have some issues though.

One is that this isn't truly a "reduced cost" any more, at least in terms of what that means from a linear programming perspective. If we're moving towards using this approach for all assets (candidates and existing), then I think we should stop using the term reduced cost (I always found this term confusing anyway). It's sort of an "activity cost", but:

The second issue is that, as this now has to take the agent objective into account, I think we'd be better off doing this calculation within the investment algorithm. The process would be: perform dispatch -> calculate commodity prices -> pass these prices to investment algorithm -> calculate_coefficients_for_lcox would take these prices and calculate an appropriate activity cost using these prices, rather than the current approach of using reduced costs (we'd need to modify activity_cost()), which in this case would ignore flows from the primary output -> I think the rest is the same.

Is there any reason this wouldn't work that I'm missing?

Perhaps this would end up unnecessarily repeating calculations so we should reorder things a bit. In any case, I think we should move away from the current approach of calculating "reduced costs" within the dispatch stage and passing these to the investment algorithm, especially when the meaning of these costs can change due to agent decisions unrelated to dispatch

Furthermore, at some point we're going to allow agents to have multiple objectives (maybe LCOX and NPV), so the approach here wouldn't work for that

I think this is probably fine for now and maybe worth merging as an intermediate. I do have some issues though.

One is that this isn't truly a "reduced cost" any more, at least in terms of what that means from a linear programming perspective. If we're moving towards using this approach for all assets (candidates and existing), then I think we should stop using the term reduced cost (I always found this term confusing anyway). It's sort of an "activity cost", but:

Happy to stop using the term... I guess if we do this, we should update the docs for consistency. What do you think about this @ahawkes?

The second issue is that, as this now has to take the agent objective into account, I think we'd be better off doing this calculation within the investment algorithm. The process would be: perform dispatch -> calculate commodity prices -> pass these prices to investment algorithm -> calculate_coefficients_for_lcox would take these prices and calculate an appropriate activity cost using these prices, rather than the current approach of using reduced costs (we'd need to modify activity_cost()), which in this case would ignore flows from the primary output -> I think the rest is the same.

Is there any reason this wouldn't work that I'm missing?

Perhaps this would end up unnecessarily repeating calculations so we should reorder things a bit. In any case, I think we should move away from the current approach of calculating "reduced costs" within the dispatch stage and passing these to the investment algorithm, especially when the meaning of these costs can change due to agent decisions unrelated to dispatch

I think the problem is that you would end up recomputing reduced costs, as you say. We could potentially move things around a bit though; maybe the code for reduced costs (or whatever we want to call them) shouldn't live in prices.rs.

Can you elaborate on your last point? When would the meaning of the costs change?

Furthermore, at some point we're going to allow agents to have multiple objectives (maybe LCOX and NPV), so the approach here wouldn't work for that

Yes, but that's a future us problem 😉. Users can't do that now and if we decided we wanted to allow it, this would be one of the problems we'd need a solution to first. It's worth noting that the code was wrong before, so this is technically just a bugfix.

Yes please stop using the term reduced costs. Intuition is that using them will never work, judging by what we’re seeing.

Okey doke. What would be a good term to use instead? I suppose it's essentially profit per unit activity... (though in the LCOX case we're excluding the COI). I'll hold off on changing the terminology until we do #879 and update the docs (which also use the term "reduced costs" to describe the alternative calculation for existing assets) at the same time.

Re the rest of this debate. I agree that all investment related calculations should reside in the investment part of the code. So yes, pass prices there, and then do calcs. To keep code well placed for future updates, this should work better, as future objectives can be built in the same place, manipulating prices in other ways.

I could move the code for "not quite reduced costs" (or whatever we're calling it) to somewhere else, but I'm not sure it matters much. Bear in mind this is the first PR in a series and we'll probably reorganise things more as we go along anyway.

@tsmbland In terms of separating out the calculations for LCOX and NPV, I don't think that's a good idea at this stage. Currently, assets either need the LCOX-type calculation or the NPV-type one, but not both, and we have a map that's keyed by asset, so you'll always have the right value for a given asset. Let's not overcomplicate things.

Yes please stop using the term reduced costs. Intuition is that using them will never work, judging by what we’re seeing.

Okey doke. What would be a good term to use instead? I suppose it's essentially profit per unit activity... (though in the LCOX case we're excluding the COI). I'll hold off on changing the terminology until we do #879 and update the docs (which also use the term "reduced costs" to describe the alternative calculation for existing assets) at the same time.

I don't think we have to/should give this a name. It's essentially just an intermediate part of the LCOX or NPV calculation that doesn't have any meaning on its own

Re the rest of this debate. I agree that all investment related calculations should reside in the investment part of the code. So yes, pass prices there, and then do calcs. To keep code well placed for future updates, this should work better, as future objectives can be built in the same place, manipulating prices in other ways.

I could move the code for "not quite reduced costs" (or whatever we're calling it) to somewhere else, but I'm not sure it matters much. Bear in mind this is the first PR in a series and we'll probably reorganise things more as we go along anyway.

@tsmbland In terms of separating out the calculations for LCOX and NPV, I don't think that's a good idea at this stage. Currently, assets either need the LCOX-type calculation or the NPV-type one, but not both, and we have a map that's keyed by asset, so you'll always have the right value for a given asset. Let's not overcomplicate things.

Sure. My idea is that we should pass prices to the investment algorithm, then calculate CoefficientsMap for each asset upfront using these prices, then continue with appraisal. I think that's better than how we currently do it as we're calculating CoefficientsMap multiple times when (I don't think) it will change.

Maybe worth chatting about this. In the end it may be better to merge this then change it later, or maybe better to take a more direct route

Yes please stop using the term reduced costs. Intuition is that using them will never work, judging by what we’re seeing.

Okey doke. What would be a good term to use instead? I suppose it's essentially profit per unit activity... (though in the LCOX case we're excluding the COI). I'll hold off on changing the terminology until we do #879 and update the docs (which also use the term "reduced costs" to describe the alternative calculation for existing assets) at the same time.

I don't think we have to/should give this a name. It's essentially just an intermediate part of the LCOX or NPV calculation that doesn't have any meaning on its own

Fair enough. Put simply it's the "LCOX related total variable operating cost" - but indeed just an intermediate of LCOX, and specific to LCOX. Though - as an aside - worth noting that this "thing" we're calculating may end up being an objective in itself (e.g a lexicographic strategy that balances LCOX against total variable operating cost - i.e. it's cheap in the long term, and it's also likely to get dispatched).

Re the rest of this debate. I agree that all investment related calculations should reside in the investment part of the code. So yes, pass prices there, and then do calcs. To keep code well placed for future updates, this should work better, as future objectives can be built in the same place, manipulating prices in other ways.

I could move the code for "not quite reduced costs" (or whatever we're calling it) to somewhere else, but I'm not sure it matters much. Bear in mind this is the first PR in a series and we'll probably reorganise things more as we go along anyway.
@tsmbland In terms of separating out the calculations for LCOX and NPV, I don't think that's a good idea at this stage. Currently, assets either need the LCOX-type calculation or the NPV-type one, but not both, and we have a map that's keyed by asset, so you'll always have the right value for a given asset. Let's not overcomplicate things.

I guess it's unlikely we'd have both LCOX and NPV for one agent/commodity. But for sure in future it's possible to have multiple objectives, and are combined via a decision rule, just like MUSE1. Not sure that helps here, but if it does and code can be structured for this addition accordingly that would be great.

Sure. My idea is that we should pass prices to the investment algorithm, then calculate CoefficientsMap for each asset upfront using these prices, then continue with appraisal. I think that's better than how we currently do it as we're calculating CoefficientsMap multiple times when (I don't think) it will change.

Maybe worth chatting about this. In the end it may be better to merge this then change it later, or maybe better to take a more direct route

Sure. My idea is that we should pass prices to the investment algorithm, then calculate CoefficientsMap for each asset upfront using these prices, then continue with appraisal. I think that's better than how we currently do it as we're calculating CoefficientsMap multiple times when (I don't think) it will change.

Maybe worth chatting about this. In the end it may be better to merge this then change it later, or maybe better to take a more direct route

@tsmbland Ohhh I see. Sorry, I hadn't clocked that... yes, that make sense. I think it probably makes sense to merge this PR in the short term though; it's a relatively simple fix as is and I'd like to try to sort out #876 ASAP. But we should open an issue about restructuring the code along those lines and circle back to it.

@ahawkes Agree with what you're saying. We'll try to keep the code modular.

Sure. My idea is that we should pass prices to the investment algorithm, then calculate CoefficientsMap for each asset upfront using these prices, then continue with appraisal. I think that's better than how we currently do it as we're calculating CoefficientsMap multiple times when (I don't think) it will change.
Maybe worth chatting about this. In the end it may be better to merge this then change it later, or maybe better to take a more direct route

@tsmbland Ohhh I see. Sorry, I hadn't clocked that... yes, that make sense. I think it probably makes sense to merge this PR in the short term though; it's a relatively simple fix as is and I'd like to try to sort out #876 ASAP. But we should open an issue about restructuring the code along those lines and circle back to it.

@ahawkes Agree with what you're saying. We'll try to keep the code modular.

Ok sounds good. I believe this is all correct so I'll approve. I'd like to do this restructuring ASAP as well, but don't want to step on your toes, so please let me know when you're done with all the "reduced costs" stuff from your side

@tsmbland: Ok let's merge this now and then if you want to create an issue for the refactoring and assign yourself to it, that'd be great!

I don't think it'll conflict with anything else I'm doing right now, so feel free to crack on 😄

@tsmbland: Ok let's merge this now and then if you want to create an issue for the refactoring and assign yourself to it, that'd be great!

#885

I guess this will also by nature fix #879

If you want to do both at the same time, be my guest! It probs makes more sense than trying to split them up.

If you want to do both at the same time, be my guest! It probs makes more sense than trying to split them up.

True. I guess there are two steps:

1. Refactor so the coefficients are calculated upfront
2. Retire the concept of "reduced costs", passing prices directly to the coefficients calculation

Point 2 will have to fix #879, but I can do point 1 first separately

Totally up to you. If it's easy enough to do in one PR that's fine, but feel free to split up if it's getting too long or you want feedback or whatever.